Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[WIP] Convert Disk APIs implementation to syscalls #201

Closed
wants to merge 3 commits into from
Closed

[WIP] Convert Disk APIs implementation to syscalls #201

wants to merge 3 commits into from

Conversation

pradeep-hegde
Copy link
Contributor

What type of PR is this?
/kind feature

What this PR does / why we need it:
Attempt to convert Disk APIs implementation to syscall alternatives

Which issue(s) this PR fixes:

Special notes for your reviewer:
Its WIP change to attempt to convert PowerShell commands to syscalls for Disk APIs. Few APIs of syscall are not running as expected. Reviews and suggestions are welcomed

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 24, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: pradeep-hegde
To complete the pull request process, please assign jsafrane after the PR has been reviewed.
You can assign the PR to them by writing /assign @jsafrane in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot
Copy link
Contributor

Hi @pradeep-hegde. Thanks for your PR.

I'm waiting for a kubernetes-csi member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 24, 2022
@pradeep-hegde pradeep-hegde marked this pull request as draft February 24, 2022 05:56
@pradeep-hegde pradeep-hegde marked this pull request as ready for review February 25, 2022 09:19
@xing-yang
Copy link

/assign @jingxu97

@xing-yang
Copy link

/assign @mauriciopoppe

@xing-yang
Copy link

/assign @ddebroy

@mauriciopoppe
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 3, 2022
mauriciopoppe
mauriciopoppe reviewed Mar 3, 2022
View reviewed changes
Copy link
Member

@mauriciopoppe mauriciopoppe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see a few TODOs in the PR, is it still a WIP?

@pradeep-hegde
Copy link
Contributor Author

I see a few TODOs in the PR, is it still a WIP?

Yes, it's WIP. Without those fixes, APIs won't be ready with the new implementation. I am unsure how to fix those and still looking for a solution to it. So raised PR to get initial reviews and suggestions for those TODOs

mauriciopoppe
mauriciopoppe reviewed Mar 3, 2022
View reviewed changes
Copy link
Member

@mauriciopoppe mauriciopoppe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar with Windows syscalls

  • how did you get the code? did you translate it from an existing codebase in a different language?
  • none of these functions are used in the API, how do we make sure that they're working?

@pradeep-hegde
Copy link
Contributor Author

  • how did you get the code? did you translate it from an existing codebase in a different language?

I referred to a few existing DiskAPIs in CSI proxy

csi-proxy/pkg/os/disk/api.go

Line 180 in 713bbca

func (DiskAPI) GetDiskNumber(disk syscall.Handle) (uint32, error) {

There is Windows Doc for all supported Disk APIs through DeviceIoControl function with ioControlCodes
https://docs.microsoft.com/en-us/windows/win32/fileio/disk-management-control-codes
@ddebroy helped to get some of the windows docs for ioControlCodes

  • none of these functions are used in the API, how do we make sure that they're working?

Yes, I am trying to have similar functions of Disk APIs with syscall implementation. Before raising PR, I tried to unit test each syscall in Windows Server 2019 to validate the functions
I am not sure if there are unit tests that can validate PowerShell alternative functions. I can give it a try.
Idea is to have the same APIs working with a new implementation. Do you think it's better to create a new API instead?

@mauriciopoppe
Copy link
Member

Idea is to have the same APIs working with a new implementation. Do you think it's better to create a new API instead?

Agreed on using a new impl with the existing APIs, creating new APIs is painful and I don't think it's worth it at this point, you could change the impl call pkg/server/disk/server.go or forward the call through the existing APIs in pkg/os/disk/api.go

I am not sure if there are unit tests that can validate PowerShell alternative functions. I can give it a try.

We have a few integration tests for disks that run in pull-kubernetes-csi-csi-proxy-integration, when the job runs after the impl is changed please look at the output, while we tried to have a good coverage I'm not sure about an exact number, please look at integrationtests/disk_v1_test.go

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 2, 2022
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jul 2, 2022
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue or PR with /reopen
  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

@k8s-ci-robot
Copy link
Contributor

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue or PR with /reopen
  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

humblec added a commit to humblec/csi-proxy that referenced this pull request Sep 23, 2022
@humblec
Squashed 'release-tools/' changes from 7fe5149..d24254f
6628f60 
d24254f Merge pull request kubernetes-csi#202 from xing-yang/kind_0.14.0
0faa3fc Update to Kind v0.14.0 images
ef4e1b2 Merge pull request kubernetes-csi#201 from xing-yang/add_1.24_image
4ddce25 Add 1.24 Kind image

git-subtree-dir: release-tools
git-subtree-split: d24254f6aa780bb6ba36a946973ee01df5633f6b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mauriciopoppe mauriciopoppe mauriciopoppe left review comments

@Jiawei0227 Jiawei0227 Awaiting requested review from Jiawei0227

@pohly pohly Awaiting requested review from pohly

@chrishenzie chrishenzie Awaiting requested review from chrishenzie

@xing-yang xing-yang Awaiting requested review from xing-yang

Assignees

@mauriciopoppe mauriciopoppe

@jingxu97 jingxu97

@ddebroy ddebroy

Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@pradeep-hegde @k8s-ci-robot @xing-yang @mauriciopoppe @k8s-triage-robot @jingxu97 @ddebroy